Skip to content

fix(vm): combinations on lazy iterators skipped tail combinations 🔢 - #144

Merged
timfennis merged 1 commit into
masterfrom
bugfix/combinations-lazy-source
May 24, 2026
Merged

fix(vm): combinations on lazy iterators skipped tail combinations 🔢#144
timfennis merged 1 commit into
masterfrom
bugfix/combinations-lazy-source

Conversation

@timfennis

Copy link
Copy Markdown
Owner

What

CombinationsIter's pivot algorithm gave incorrect results for lazy sources (any iterator that buffers on demand, e.g. the output of .enumerate(), .map(), .filter(), etc.).

Reproducer

print([1, 2, 3, 4].combinations(2).list());
// [(1,2),(1,3),(1,4),(2,3),(2,4),(3,4)]    ← correct

print([1, 2, 3, 4].enumerate().combinations(2).list());
// [((0,1),(1,2)),((0,1),(2,3)),((1,2),(2,3)),((1,2),(3,4)),((2,3),(3,4))]
//                                ^ ((0,1),(3,4)) is missing and combinations are out of lex order

Eager sources (a list directly) were unaffected. Lazy sources missed combinations whose rightmost index landed in the not-yet-pulled tail.

Cause

CombinationsIter keeps a growing buffer of pulled elements and a list of indices into that buffer. To produce the next combination it picks the rightmost index that can still advance within the current buffer:

let pivot = match (0..k).rev().find(|&i| self.indices[i] < pool_len - k + i) { ... };

For an eager source, pool_len is the final size and the check is correct. For a lazy source, pool_len is "what's been pulled so far". When indices[k-1] reaches the end of the current buffer, the algorithm decides "no room to advance the rightmost index" and falls back to advancing an earlier index — even though one more pull would have unlocked it.

The existing code did try to pull more, but only in the fallback pivot = None branch, which only fires when every index is at its current-pool maximum. In our trace above the algorithm picks pivot = 0 before reaching that fallback.

Fix

Pull indices[k - 1] + 1 worth of buffer before computing the pivot. If the source still has elements, pool_len grows and the rightmost index pivots as expected. If the source is exhausted the buffer stays put and the existing pivot logic correctly falls back to an earlier index.

Tests

  • New tests/functional/programs/900_bugs/bug0021_combinations_lazy_source.ndc covering the original 4-element case, the larger k = 3 case, and the eager-source baseline.
  • All 287 existing functional tests pass.

🤖 Generated with Claude Code

CombinationsIter's pivot algorithm assumed `self.buffer.len()` reflects
the full pool, but for lazy sources the buffer is filled on demand.
When `indices[k-1]` reached the current buffer end the algorithm would
declare the rightmost index "at max" against the not-yet-extended pool
and advance an earlier index instead — yielding combinations out of lex
order and skipping ones whose rightmost index lived in the unpulled
tail.

Concretely, `[1, 2, 3, 4].enumerate().combinations(2)` yielded five
combinations instead of six, missing `((0, 1), (3, 4))`. Eager sources
(`[...].combinations(...)`) were unaffected.

Fix: pull `indices[k-1] + 1` before computing pivot. The rightmost index
then pivots whenever the source still has elements, matching eager
ordering. When pulls fail the buffer stays as-is and the pivot logic
correctly falls back to advancing an earlier index.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@timfennis
timfennis merged commit cf6dac0 into master May 24, 2026
1 check passed
@timfennis
timfennis deleted the bugfix/combinations-lazy-source branch May 24, 2026 06:56
timfennis added a commit that referenced this pull request May 24, 2026
After #144 landed, master had two `bug0021_*` files —
`bug0021_chained_vectorized_tuple_arith.ndc` (from the original #140
work, in master longer) and `bug0021_combinations_lazy_source.ndc`
(just merged via #144). Renumber the combinations test to bug0022
since chained-vectorized got there first, and bump this branch's
incompat-Dynamic test to bug0023.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant